-
Notifications
You must be signed in to change notification settings - Fork 393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(engine-core): dynamic class with empty string results in hydration mismatch #4684
fix(engine-core): dynamic class with empty string results in hydration mismatch #4684
Conversation
@@ -664,7 +664,7 @@ function validateClassAttr( | |||
|
|||
const elmClassName = getAttribute(elm, 'class'); | |||
|
|||
if (!isUndefined(className) && String(className) !== elmClassName) { | |||
if (!isUndefined(className) && String(className) !== elmClassName && className !== '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't totally right. If the client class name is ''
but the server class name is 'foo'
then there will be no mismatch when there should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: #4698
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I figured some case wasn't covered here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see above
Thanks @nolanlawson . Should I try fixing this once you merge the new tests? Or are you taking care of it as well? |
@cardoso Feel free to go ahead! If you don't get to it, I'll get to it eventually. 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the logic, tests are passing @nolanlawson .
if (!isUndefined(className) && String(className) !== elmClassName) { | ||
if ( | ||
!isUndefined(className) && | ||
String(className) !== elmClassName && | ||
// No mismatch if SSR className is empty and the client-side class is null | ||
!(className === '' && isNull(elmClassName)) | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if is starting to get hairy, but the auto-formatting gave me the idea of adding this comment. I think this is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also be (className !== '' || !isNull(elmClassName))
but I found this way more readable and easy to explain via the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you. 🙇
/nucleus test |
/nucleus test |
Details
Hopefully this is right, but if not there should be some test that fails.
Closes #4661
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
No hydration mismatch on dynamic class with empty string 😄
GUS work item